The Race Condition

The bug lives in startPeriodicPing() at packages/core/src/shared/protocol.ts around lines 778-804. The setInterval callback is async, so once it fires and awaits _requestWithSchema(), JavaScript cannot cancel it. Clearing the interval only prevents future firings; the already-started async function continues running. This is the core of the race.

Exact Code Path

1. The setInterval fires. The async callback begins executing and calls _requestWithSchema({ method: 'ping' }, ...), which registers a response handler in _responseHandlers and suspends at the await.
2. Before the server responds, the caller invokes close().
3. close() calls stopPeriodicPing(), which does clearInterval(this._pingTimer). The in-flight async callback has already started; clearing the interval only prevents future firings.
4. close() then calls this._transport?.close(), which triggers the transport onclose callback.
5. That callback invokes _onclose(), which iterates over all entries in _responseHandlers and rejects each one with new SdkError(SdkErrorCode.ConnectionClosed, 'Connection closed').
6. The pending _requestWithSchema promise now rejects with the ConnectionClosed error.
7. The catch block in startPeriodicPing executes: since SdkError extends Error, this._onerror(error) fires.
8. The user's onerror callback receives a SdkError(ConnectionClosed) during a normal, user-initiated shutdown.

Why Existing Code Does Not Prevent It

The close() method does call stopPeriodicPing() before this._transport?.close(), which looks like it should protect against this. But stopPeriodicPing() only prevents new timer firings. It cannot retroactively cancel an async function that has already started and is awaiting a promise. There is no mechanism in the ping catch block to distinguish intentional close from a genuine connection failure.

Impact

Any consumer that sets onerror to monitor connection health (the primary intended use case per the PR description) will see false positive ConnectionClosed errors every time close() is called while a ping happens to be in-flight. Since pings fire on a regular interval, the race is probabilistic but reliably reproducible at scale or in tests. More critically, these errors are indistinguishable from a real remote-side failure, so automated reconnection logic or alerting built on onerror will misfire.

How to Fix

Add a private _closing boolean field to Protocol, initialized to false. In close(), set this._closing = true before calling stopPeriodicPing() and this._transport?.close(). In the ping catch block, guard the onerror call:

} catch (error) {
    if (!this._closing) {
        this._onerror(error instanceof Error ? error : new Error('Periodic ping failed: ' + String(error)));
    }
}

Reset _closing to false in _onclose() after cleanup so that a reconnected protocol instance works correctly. The _closing flag cleanly suppresses the error regardless of what exact error code the transport produces on shutdown.

Step-by-Step Proof

T=0   setInterval fires; pingCallback() begins executing
T=0   _requestWithSchema registers handler H in _responseHandlers, yields at await
T=1   close() is called by user
T=1   stopPeriodicPing() clears the timer -- in-flight pingCallback is unaffected
T=1   this._transport.close() called
T=1   transport triggers onclose -> _onclose() runs
T=1   _onclose(): for each handler in responseHandlers -> handler(SdkError(ConnectionClosed))
T=1   Handler H rejects; pingCallback resumes in catch block
T=1   catch(error): this._onerror(error) -- SPURIOUS ERROR DELIVERED TO USER
T=1   user onerror fires with 'Connection closed' during intentional shutdown

What the bug is

In startPeriodicPing() (protocol.ts:772-780), setInterval fires every _pingIntervalMs regardless of whether the previous async callback has finished. The ping timeout passed to _requestWithSchema is also this._pingIntervalMs. This means the internal setTimeout that will reject ping N with a timeout error and the next setInterval tick that fires ping N+1 are both scheduled for the same wall-clock time (T + pingIntervalMs from ping N start). Which fires first is non-deterministic: it depends on the JS engine timer queue ordering, which is not specified.

The specific code path

The relevant code:

this._pingTimer = setInterval(async () => {
    try {
        await this._requestWithSchema({ method: 'ping' }, EmptyResultSchema, {
            timeout: this._pingIntervalMs  // same value as the interval
        });
    } catch (error) {
        this._onerror(...);
    }
}, this._pingIntervalMs);

Because setInterval does not await the async callback, it fires the next tick at T + pingIntervalMs unconditionally. The request inside the callback has its own setTimeout-based timeout also set to pingIntervalMs. Both macrotasks are enqueued for approximately the same wall-clock time.

Why existing code does not prevent it

The idempotency guard only prevents duplicate timers from multiple startPeriodicPing() calls. It does nothing to serialize individual ping invocations. Once the interval is running, each tick creates a new promise/request that executes concurrently with any still-in-flight ping.

Addressing the refutation and impact

The refutation correctly notes that the overlap is bounded: because the timeout equals the interval, each lingering ping will be canceled at approximately the same time the next one starts. The worst case is 2 concurrent pings, not N. In practice, on a well-functioning connection, pings respond well within the interval and there is no overlap. Only when a ping is slow (response time approaching pingIntervalMs) does the race condition manifest, and even then it is transient and bounded.

However, the PR description explicitly claims: "Ping timeout equals the interval: if a ping takes longer than one interval to respond, it fails with a timeout error rather than stacking up." This is imprecise. Due to timer imprecision, the new ping can start before the old one is cleaned up, meaning they do briefly stack up (2 at once). The stated guarantee is not actually enforced by the implementation.

Step-by-step proof

1. T=0: setInterval fires, ping Better validation of data coming over the wire #1 is sent. A setTimeout(reject, pingIntervalMs) is registered inside _requestWithSchema.
2. T=pingIntervalMs: Both the interval tick (ping Race condition between transport start and server connection #2) and the timeout for ping Better validation of data coming over the wire #1 are queued as macrotasks. Two timers set for the same target time have implementation-defined ordering.
3. If the interval fires first: ping Race condition between transport start and server connection #2 is sent and added to _responseHandlers while ping Better validation of data coming over the wire #1's handler is still present. Both are in-flight simultaneously for a brief moment before ping Better validation of data coming over the wire #1's timeout fires.
4. If the timeout fires first: ping Better validation of data coming over the wire #1 is cleaned up, then ping Race condition between transport start and server connection #2 is sent — matching the documented behavior.

How to fix it

Replace setInterval with a self-rescheduling setTimeout pattern that only schedules the next ping after the current one completes (success or failure). This strictly serializes pings and matches the documented one-at-a-time intent, at the cost of the interval becoming pingIntervalMs + RTT rather than exactly pingIntervalMs — an acceptable trade-off for health monitoring.

What the bug is

The JSDoc comment on startPeriodicPing() (protocol.ts lines 759-762) contains a false statement: "This is called automatically at the end of connect() when pingIntervalMs is set." This implies the base Protocol.connect() is responsible for starting the ping timer when the option is configured.

The specific code path

Looking at Protocol.connect(), the method sets up transport callbacks and ends with await this._transport.start(). There is no call to startPeriodicPing() anywhere in the base implementation. The actual callers are: (1) Client.connect() calls this.startPeriodicPing() after the full MCP initialization handshake completes (and also on the reconnection fast-path); (2) Server._handleInitialized() calls this.startPeriodicPing() when the notifications/initialized notification is received.

Why existing code does not prevent the confusion

The second sentence of the JSDoc partially acknowledges the real behavior: "Subclasses that override connect() and perform additional initialization may call this method after their initialization is complete instead." However, this reads as an opt-out from the stated automatic behavior, not a correction of it. The false first sentence still stands.

Impact

Any developer implementing a custom Protocol subclass who passes pingIntervalMs and does not override connect() will get no periodic pings, despite the documentation promising automatic invocation. The ping timer simply never starts. This is a silent failure — no error is thrown, and _pingIntervalMs is set, so there is no obvious indication that something is wrong.

Step-by-step proof

1. Create a class extending Protocol without overriding connect()
2. Instantiate with new MyProtocol({ pingIntervalMs: 30_000 })
3. Call await myProtocol.connect(transport) — this runs Protocol.connect(), which calls transport.start() and returns
4. startPeriodicPing() is never invoked; _pingTimer remains undefined
5. No pings are ever sent, despite _pingIntervalMs being set to 30,000

Fix

Update the JSDoc to accurately state that Client and Server are the actual callers, and that custom subclasses must call this method explicitly after their own initialization is complete.

Regarding the duplicate refutation

One verifier flagged this as a duplicate of bug_004. Whether or not another report covers the same issue, the bug is independently confirmed and real. The JSDoc is factually wrong regardless of duplication — the duplication concern is a process question, not a technical refutation.